-
Notifications
You must be signed in to change notification settings - Fork 91
feat: support revalidateTag with SWR behavior #3173
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: support revalidateTag with SWR behavior #3173
Conversation
📊 Package size report 0.06%↑
Unchanged files
🤖 This report was automatically generated by pkg-size-action |
5be6bef
to
4da8739
Compare
80e143c
to
e69ecd6
Compare
e69ecd6
to
e0a4e48
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💯 LGTM, just some nitpicks and minor suggestions
src/run/handlers/tags-handler.cts
Outdated
const timestampsOrNulls = await Promise.all(tags.map((tag) => getTagManifest(tag, cacheStore))) | ||
|
||
const timestamps = timestampsOrNulls.filter((timestamp) => timestamp !== null) | ||
if (timestamps.length === 0) { | ||
const expirationTimestamps = timestampsOrNulls |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
left over from refactor:
const timestampsOrNulls = await Promise.all(tags.map((tag) => getTagManifest(tag, cacheStore))) | |
const timestamps = timestampsOrNulls.filter((timestamp) => timestamp !== null) | |
if (timestamps.length === 0) { | |
const expirationTimestamps = timestampsOrNulls | |
const manifestsOrNulls = await Promise.all(tags.map((tag) => getTagManifest(tag, cacheStore))) | |
const expirationTimestamps = manifestsOrNulls |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
github didn't allow me to commit suggestion so this was done as part of 37c3e8e#diff-7fe4e66d51e744fc6fd6321c633def547066010bb05aff0ef621b64d84ec5442R35
src/run/handlers/tags-handler.cts
Outdated
getLogger().withFields({ tags }).debug('doRevalidateTagAndPurgeEdgeCache') | ||
async function doRevalidateTagAndPurgeEdgeCache( | ||
tags: string[], | ||
durations?: { expire?: number }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion:
durations?: { expire?: number }, | |
durations?: { expireSeconds?: number }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the durations
type is from Next.js - https://github.com/vercel/next.js/blob/fffa2831b61fa74852736eeaad2f17fbdd553bce/packages/next/src/server/lib/incremental-cache/index.ts#L78
I do not need to use the exact type here, but I do need to use original type at least in our CacheHandler and would need to add conversion from Next.js type to our type which doesn't seem worth it?
But for clarity I will add some jsdocs or code comments along the call chain to clarify that expire
is in seconds
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added type with some descriptions 8f0a2d4
src/run/handlers/tags-handler.cts
Outdated
export function markTagsAsStaleAndPurgeEdgeCache(tagOrTags: string | string[]) { | ||
export function markTagsAsStaleAndPurgeEdgeCache( | ||
tagOrTags: string | string[], | ||
durations?: { expire?: number }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion:
durations?: { expire?: number }, | |
durations?: { expireSeconds?: number }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same as #3173 (comment)
if (nextVersionSatisfies('>=16.0.0-alpha.0')) { | ||
test.describe('app router on-demand revalidation (Next 16 APIs)', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
since these tests will be relatively slow with all the waiting and they should be independent, maybe we could parallelize them?
if (nextVersionSatisfies('>=16.0.0-alpha.0')) { | |
test.describe('app router on-demand revalidation (Next 16 APIs)', () => { | |
test.describe('app router on-demand revalidation (Next 16 APIs)', () => { | |
test.describe.configure({ mode: 'parallel' }) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
github didn't allow me to commit suggestion so I did it in e13e932
Co-authored-by: Philippe Serhal <philippe.serhal@netlify.com>
a4b1046...feat/support-update-tag-and-revalidate-tag-second-arg for the changes since review (including applied suggestions) |
Description
To support https://nextjs.org/blog/next-16-beta#improved-caching-apis
Note that
updateTag
is supported without any changes (I just added test cases for it). The runtime changes are to supportrevaligateTag
with SWR behaviorDocumentation
Tests
Will be added
Relevant links (GitHub issues, etc.) or a picture of cute animal